CNS should not mark itself as ready if it can't write out a conflist. #4146
Conversation
cns/restserver/synchostnc.go
Outdated
|
|
||
| // SyncHostNCVersion will check NC version from NMAgent and save it as host NC version in container status. | ||
| // If NMAgent NC version got updated, CNS will refresh the pending programming IP status. | ||
| func (service *HTTPRestService) SyncHostNCVersion(ctx context.Context, channelMode string) { |
There was a problem hiding this comment.
this and syncHostNCVersion are unchanged.
There was a problem hiding this comment.
Pull request overview
This PR introduces a synchronization mechanism to prevent CNS from marking itself as ready before the CNI conflist has been successfully generated. It refactors the SyncHostNCVersion loop code by extracting it into a new file and adds a WaitForNCSynced function that blocks until the conflist is written out.
Key changes:
- Added
WaitForNCSyncedcall in main.go before marking the service as ready - Refactored duplicate SyncHostNCVersion loop code into a new
StartSyncHostNCVersionLoopfunction - Moved sync-related functions to a new file
synchostnc.gofor better code organization
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| cns/service/main.go | Adds WaitForNCSynced call before marking service ready; refactors duplicate SyncHostNCVersion loop initialization to use StartSyncHostNCVersionLoop |
| cns/restserver/synchostnc.go | New file containing refactored sync functionality including StartSyncHostNCVersionLoop, SyncHostNCVersion, syncHostNCVersion, MustGenerateCNIConflistOnce, and WaitForNCSynced |
| cns/restserver/restserver.go | Adds ncSynced channel field; removes MustGenerateCNIConflistOnce (moved to synchostnc.go) |
| cns/restserver/internalapi.go | Removes SyncHostNCVersion and syncHostNCVersion functions (moved to synchostnc.go); removes unused imports |
| cns/healthserver/healthz.go | Minor formatting improvement (adds blank line) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
cns/restserver/synchostnc.go
Outdated
| service.generateCNIConflistOnce.Do(func() { | ||
| close(service.ncSynced) |
There was a problem hiding this comment.
The ncSynced channel is closed before the conflist generation completes. If Generate() or Close() fail and panic, the channel will already be closed, potentially allowing WaitForNCSynced to return success even though the conflist generation failed. Consider moving close(service.ncSynced) to after both Generate() and Close() succeed, or handle it in a defer that checks if the operations succeeded.
cns/restserver/synchostnc.go
Outdated
|
|
||
| func (service *HTTPRestService) StartSyncHostNCVersionLoop(ctx context.Context, cnsconfig configuration.CNSConfig) { | ||
| //do we need a sync.once to protect this? should we error if this is called twice? | ||
| service.ncSynced = make(chan struct{}) |
There was a problem hiding this comment.
The ncSynced channel is unconditionally recreated without protection. If StartSyncHostNCVersionLoop is called multiple times (which could happen in error scenarios or test code), it will create a new channel and orphan any goroutines waiting on the previous channel. Consider using sync.Once to ensure the channel is only initialized once, or return an error if it's already initialized, as suggested by the comment on line 21.
cns/restserver/nodesubnet.go
Outdated
| // saved NC successfully. UpdateIPsForNodeSubnet is called only when IPs are fetched from NMAgent. | ||
| // We now have IPs to serve IPAM requests. Generate conflist to indicate CNS is ready | ||
| service.MustGenerateCNIConflistOnce() | ||
| service.ncSynced = make(chan struct{}) // in case this is called multiple times |
There was a problem hiding this comment.
Also can revert though maybe a commetn here is good.
cns/healthserver/healthz.go
Outdated
| return nil | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
revert or push ready channel into here.
Co-authored-by: Copilot <[email protected]> Signed-off-by: Paul Miller <[email protected]>
1 participant
Reason for Change:
Issue Fixed:
Requirements:
Notes: